Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: p11prov_tls_constant_time_depadding bug corrected #438

Closed
wants to merge 2 commits into from
Closed

fix: p11prov_tls_constant_time_depadding bug corrected #438

wants to merge 2 commits into from

Conversation

sebastienandert
Copy link

Hi,

Today, the connection to EVIDEN’s HSM CRYPT2PAY using the Latchset provider fails when the openssl “-cipher” option (any TLS_RSA encryption) is requested (e.g. ./openssl s_client -connect 127.0.0.1:11036 -CApath /int1/pki/store/ac/crt_rehash -tls1_2 -cipher AES128-GCM-SHA256)
The problem is caused by the 'p11prov_tls_constant_time_depadding' function in the source file src/asymmetric_cipher. c, lines 274 and 289.

274: cond = constant_equal(*out_size, 2 + length);
...
289: constant_select_buf (conc, length, out, buf + 2, randbuf);

the '2' offset is a problem and corresponds to the first 2 bytes before padding removal (done by PKCS#11)
The best proof is lines 275 and 277 which compare 2 first bytes of input buffer with client version (major and minor)

Tests :

This corrected version has been tested in our testing environment and works correctly in all our use cases.
Could it be taken into account ?

Thanks

@Jakuje
Copy link
Contributor

Jakuje commented Sep 3, 2024

Thank you for testing the functionality and coming back with the results!

The tests now fail with software tokens such this

=================================== 49/58 ==================================== 
test:         pkcs11-provider:softokn / tls
start time:   13:38:47
duration:     0.19s
result:       exit status 1
command:      MALLOC_PERTURB_=115 ASAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1 TEST_PATH=/home/jjelen/devel/pkcs11-provider/tests TESTBLDDIR=/home/jjelen/devel/pkcs11-provider/builddir/tests UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 MSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 /home/jjelen/devel/pkcs11-provider/tests/test-wrapper tls-softokn.t
----------------------------------- stdout -----------------------------------
Executing /home/jjelen/devel/pkcs11-provider/tests/ttls
  
## Test SSL_CTX creation 
----------------------------------- stderr -----------------------------------
SSL Context works!
Failed to decrypt TLS master key
  
==============================================================================

This is failure in the tlsctx test written with this functionality:

pkcs11-provider/tests/tlsctx.c

Line 90 in 0c7504e

err = EVP_PKEY_decrypt(ctx, dec, &declen, enc, enclen);

This looks like there was misunderstanding when implementing the test for this where we though that the version number should be prepended to the master key length, while it is

the first two bytes are the protocol version

(from https://docs.openssl.org/3.1/man3/EVP_PKEY_CTX_ctrl/#rsa-parameters )

So to get this merged, we will need to adjust the test to pass. With the following patch, I have currently failing tests working:

diff --git a/tests/tlsctx.c b/tests/tlsctx.c
index 882d872..2bc5c1d 100644
--- a/tests/tlsctx.c
+++ b/tests/tlsctx.c
@@ -14,7 +14,7 @@ static void test_pkcs1_with_tls_padding(void)
     EVP_PKEY_CTX *ctx;
     EVP_PKEY *prikey;
     EVP_PKEY *pubkey;
-    unsigned char plain[SSL_MAX_MASTER_KEY_LENGTH + 2] = { 0x03, 0x03, 0x01 };
+    unsigned char plain[SSL_MAX_MASTER_KEY_LENGTH] = { 0x03, 0x03, 0x01 };
     unsigned char enc[1024];
     unsigned char dec[1024];
     size_t enclen;
@@ -97,8 +97,8 @@ static void test_pkcs1_with_tls_padding(void)
     EVP_PKEY_CTX_free(ctx);
     EVP_PKEY_free(prikey);
 
-    if ((declen != sizeof(plain) - 2)
-        || (memcmp(plain + 2, dec, declen) != 0)) {
+    if ((declen != sizeof(plain))
+        || (memcmp(plain, dec, declen) != 0)) {
         fprintf(stderr, "Fail, decrypted master secret differs from input\n");
         ossl_err_print();
         exit(EXIT_FAILURE);

Please, apply these changes to this PR, rebase your changes on current master and provide the Sign-Off in the commit messages to pass the DCO (see the DCO test failure for more information how to do this).

@sebastienandert
Copy link
Author

Hi,

OK, thanks, I'm working on it right now

@sebastienandert sebastienandert deleted the pkcs11-provider_PKCS11_Crypt2Pay_Compliant branch September 4, 2024 14:45
@sebastienandert sebastienandert mentioned this pull request Sep 4, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sebastienandert @Jakuje